Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: annotate optional integer parameters with optional type #1487

Merged
merged 4 commits into from
Feb 8, 2023
Merged

fix: annotate optional integer parameters with optional type #1487

merged 4 commits into from
Feb 8, 2023

Conversation

tswast
Copy link
Contributor

@tswast tswast commented Feb 7, 2023 •
edited
Loading

I noticed in a project that uses this client that the type annotations for result are incorrect.

bigframes/dataframe.py:64: error: Argument "max_results" to "result" of "QueryJob" has incompatible type "Optional[int]"; expected "int"  [arg-type]

Fixes #509

@tswast tswast requested a review from a team February 7, 2023 15:53
@tswast tswast requested a review from a team as a code owner February 7, 2023 15:53
@tswast tswast requested a review from yirutang February 7, 2023 15:53
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/python-bigquery API. labels Feb 7, 2023
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Feb 7, 2023
@chalmerlowe
Copy link
Collaborator

General question:

what was the criteria for deciding which parameters should get the enhanced Type Annotation?

Noticed some other int parameters in various methods that did not get the same enhancement and was curious.

chalmerlowe
chalmerlowe approved these changes Feb 8, 2023
View reviewed changes
Copy link
Collaborator

@chalmerlowe chalmerlowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a general question to the comment thread.
Beyond that question, this LGTM.

@tswast
Copy link
Contributor Author

tswast commented Feb 8, 2023

what was the criteria for deciding which parameters should get the enhanced Type Annotation?
Noticed some other int parameters in various methods that did not get the same enhancement and was curious.

Our approach was: when you touch a method, add type annotations.

At the time, that meant we could also remove the type information from the docstring because it was then redundant information from what Sphinx generated and what we put in the docstring. Looks to be the same with https://cloud.google.com/python/docs/reference/bigquery/latest/google.cloud.bigquery.client.Client but I'm not 100% sure how the linking works over there since they don't use intersphinx.

@tswast tswast merged commit a190aaa into main Feb 8, 2023
@tswast tswast deleted the tswast-type-annotations branch February 8, 2023 16:05
@release-please release-please bot mentioned this pull request Feb 8, 2023
Merged
This was referenced Feb 28, 2023
Closed
Closed
Closed
abdelmegahedgoogle pushed a commit to abdelmegahedgoogle/python-bigquery that referenced this pull request Apr 17, 2023
@tswast @abdelmegahedgoogle
fix: annotate optional integer parameters with optional type (googlea…
dd21845 
…pis#1487)

* fix: annotate optional integer parameters with optional type

* remove google-cloud-core reference causing type checker issues

deps: update minimum google-cloud-core  to 1.6.0
@tswast tswast mentioned this pull request Nov 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalmerlowe chalmerlowe chalmerlowe approved these changes

@yirutang yirutang Awaiting requested review from yirutang

Assignees
No one assigned
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

increase minimum version of google-cloud-core to 1.6.0
2 participants
@tswast @chalmerlowe